Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Resolver] Origin process #72382

Merged
merged 9 commits into from
Jul 21, 2020
Merged

[Resolver] Origin process #72382

merged 9 commits into from
Jul 21, 2020

Conversation

oatkiller
Copy link
Contributor

@oatkiller oatkiller commented Jul 19, 2020

Co-authored-by: Brent Kimmel brent.kimmel@elastic.co
closes https://github.com/elastic/endpoint-app-team/issues/579

Summary

  • Center the origin node
  • Nodes appear selected when they are selected. also the aria attributes are working.
  • Reposition the submenu when the user pans.

Note: this doesn't include the following. We need a follow up PR

  • origin node starts off selected

Generally works:
image

Origin is in the center, selecting nodes works, and the submenu is correctly repositioned when the graph moves.

centering_and_submenu_position_and_selecting_nodes_and_aria_works mov

panels still work
panels_work mov

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Copy link
Contributor

@kqualters-elastic kqualters-elastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 code looks fine, the way the process looks not so sure

@oatkiller oatkiller force-pushed the trigger-proces branch 2 times, most recently from 12bceab to b0d689b Compare July 20, 2020 19:25
}

/**
* Return a clone of `model` with all positions incremented by `position`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ Assume by position in this comment you mean translation: Vector2 in the args as below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -94,6 +94,6 @@ async function indexAlerts(
},
[]
);
await client.bulk({ body, refresh: true });
await client.bulk({ body, refresh: 'true' });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typescript error

}

/**
* Return a clone of `model` with all positions incremented by `position`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup

}

/**
* Return a clone of `model` with all positions incremented by `position`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Return a clone of `model` with all positions incremented by `position`.
* Return a clone of `model` with all positions incremented by `translation`.

const taxiLayout = isometricTaxiLayoutModel.isometricTaxiLayoutFactory(indexedProcessTree);

if (!originID) {
// TODO, this should only happen when no data has loaded.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove TODO

return taxiLayout;
}

// subtract the origin position from the layout, centering the layout around the origin node
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO elaborate on how this works in comments

@@ -212,7 +207,7 @@ const UnstyledProcessEventDot = React.memo(
isLabelFilled,
labelButtonFill,
strokeColor,
} = cubeAssetsForNode(isProcessTerminated, isProcessOrigin);
} = cubeAssetsForNode(isProcessTerminated, false);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't defined yet. we dont have a way to know if its a trigger? TODO add comment

return {
...uiState,
activeDescendantId: action.payload.nodeId,
const next: ResolverUIState = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by using const next: ResolverUIState, typescript will complain about excess properties.

return nodeAssets[processTypeToCube.processTerminated];
} else if (isProcessOrigin) {
if (isProcessTrigger) {
return nodeAssets.terminatedTriggerCube;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

show the special terminated trigger cube.

* EuiPopover has a mutation observer that observers the child tree and repositions itself.
* Abuse this to cause it to reposition itself when the project matrix changes.
*/}
<HiddenSpan data-force-mutation-observer={projectionMatrix.join(', ')} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 still see our little experiment here.

useMemo,
useCallback,
useRef,
useLayoutEffect,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
useLayoutEffect,

CI freaked out on this

@apmmachine
Copy link
Contributor

apmmachine commented Jul 21, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #72382 updated]

  • Start Time: 2020-07-21T14:29:04.543+0000

  • Duration: 4 min 56 sec

oatkiller added 3 commits July 21, 2020 13:23
also renames 'isProcessOrigin' params in resolver theme to be
'isProcessTrigger'.

NB: this does not use the trigger node styling
UI state changes:
* rename `activeDescendantId` to `ariaActiveDescendant`. it has the
nodeID (aka entity_id) of the aria active descendant
* rename `processEntityIdOfSelectedDescendant` to `selectedNode`. it has
the nodeID (aka entity_id) of the selected node
* aria html attributes were wrong (this also effected styling.)
@oatkiller
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
securitySolution 7.3MB +1.1KB 7.3MB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@oatkiller oatkiller merged commit b930cef into elastic:master Jul 21, 2020
@oatkiller oatkiller deleted the trigger-proces branch July 21, 2020 21:47
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 22, 2020
* master: (23 commits)
  Stabilize closing toast (elastic#72097)
  stabilize failing test (elastic#72086)
  Stabilize filter bar test (elastic#72032)
  Unskip vislib tests (elastic#71452)
  [ML] Fix layout of anomaly chart tooltip for long field values (elastic#72689)
  fix preAuth/preRouting mocks (elastic#72663)
  [Security Solution] Hide KQL bar (all pages) and alerts filters (Detections) when Resolver is full screen (elastic#72788)
  [Uptime] Rename Whitelist to Allowlist in parse_filter_map (elastic#71584)
  [Security Solution] Fixes exception modal not loading content (elastic#72770)
  [Security Solution][Exceptions] - Require non empty entries and non empty string values in exception list items (elastic#72748)
  [Detections] Add validation for Threshold value field (elastic#72611)
  [SIEM][Detection Engine][Lists] Adds version and immutability data structures (elastic#72730)
  [Security Solution][Detections] Validate file type of value lists (elastic#72746)
  [pre-req] New Component Layout proposal (elastic#72385)
  [ML] do not throw an error when agg is not supported by UI (elastic#72685)
  [Resolver] Origin process (elastic#72382)
  [Ingest Manager] Allow to force unenroll from the UI (elastic#72386)
  skip 6.8 branch when triggering baseline-capture builds (elastic#72706)
  [CI] In-progress PR comments (elastic#72211)
  Fix sorting of scripted string fields (elastic#72681)
  ...
oatkiller pushed a commit to oatkiller/kibana that referenced this pull request Jul 23, 2020
Co-authored-by: Brent Kimmel <brent.kimmel@elastic.co>

* Center the origin node
* Nodes appear selected when they are selected. also the aria attributes are working.
* Reposition the submenu when the user pans.
oatkiller pushed a commit to oatkiller/kibana that referenced this pull request Jul 23, 2020
Co-authored-by: Brent Kimmel <brent.kimmel@elastic.co>

* Center the origin node
* Nodes appear selected when they are selected. also the aria attributes are working.
* Reposition the submenu when the user pans.
oatkiller pushed a commit that referenced this pull request Jul 23, 2020
Co-authored-by: Brent Kimmel <brent.kimmel@elastic.co>

* Center the origin node
* Nodes appear selected when they are selected. also the aria attributes are working.
* Reposition the submenu when the user pans.
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 23, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

oatkiller pushed a commit that referenced this pull request Jul 23, 2020
Co-authored-by: Brent Kimmel <brent.kimmel@elastic.co>

* Center the origin node
* Nodes appear selected when they are selected. also the aria attributes are working.
* Reposition the submenu when the user pans.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants